Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable error status code printing #747

Merged
merged 1 commit into from
Sep 29, 2019
Merged

Add option to disable error status code printing #747

merged 1 commit into from
Sep 29, 2019

Conversation

JacobEvelyn
Copy link
Contributor

PR #688 added functionality that broke the
JacobEvelyn/friends
build (see https://travis-ci.com/JacobEvelyn/friends/jobs/238680478),
which runs many CLI processes and parses the output. Some CLI
processes are expected to have non-success status codes, but
these tests also check that our CLI wrote the correct message
to STDERR, and with #688 the output to STDERR is now changed.

This commit adds a SimpleCov configuration flag to disable the
message added in #688.

JacobEvelyn added a commit to JacobEvelyn/friends that referenced this pull request Sep 24, 2019
This should only pass tests once
simplecov-ruby/simplecov#747 is merged and
incorporated into a new SimpleCov release.

Fixes #238
@PragTob
Copy link
Collaborator

PragTob commented Sep 24, 2019

@JacobEvelyn Hi there and thanks for the PR! 💚

Might I ask - can't you just adjust your tests to deal with the new printed exit status? Or ideally, not have simplecov fail and exit with an that error message?

I'm open to changing the message to be potentially more parsable, but the reasoning for disabling it I don't understand right now (see questions above).

@JacobEvelyn
Copy link
Contributor Author

Good questions, @PragTob! I definitely considered that, but it seemed a little hacky to me.

Context that might be helpful

How friends works

friends is a CLI Ruby app.

Typically, if you run friends <arguments> in your shell it'll do something along the lines of:

  1. Read a file.
  2. Write to the file.
  3. Print a message to STDOUT.
  4. Exit with a 0 status code.

And if you run friends <bad arguments> it'll do something along the lines of:

  1. Print a message to STDERR.
  2. Exit with a non-0 status code.

How friends is tested (+ simplecov)

The tests we use can be thought of as "black-box" integration tests; rather than test some internals of its implementation, each test in our Minitest suite actually creates a new process and runs friends <whatever> and then uses things like Open3 and File.read to see the process' output (including STDOUT, STDERR, and status code) and any file changes it made.

This testing setup simplifies a lot of things, but means that to get code coverage to work we have to have code in the CLI itself (the sub-process that each test is executing) that starts up simplecov (it uses SimpleCov.command_name Process.pid.to_s so each test doesn't wipe out the other tests' coverage data).

The problem this PR is solving

What's happening here is for our tests that are testing bad input (the friends <bad arguments> cases), when simplecov is enabled the tests fail because they're expecting STDERR to say something like Invalid argument: <bad argument> and instead they now say:

Invalid argument: <bad argument>
SimpleCov failed with exit 1

Alternatives to this PR

The way I see it, there are three obvious options to fixing our issue in friends:

1. Change friends tests to allow simplecov's output

This is I believe what you're suggesting, and while I did consider it I decided to not pursue this for a few reasons:

  1. friends is suddenly tied very closely to a specific output message of simplecov, which of course might change in the future and break our tests again. This feels a little hacky to me.
  2. It's not entirely straightforward to do this, since we run tests both with and without simplecov. We'd need to add the complexity of ignoring the simplecov message if it's present but not depending on it being there.
  3. Any other simplecov users out there who might run into the same issue will need to roll their own solutions.

2. Add a configuration option that allows disabling the #688 message

This seemed relatively simple and straightforward, and is this PR.

3. Add a configuration option that allows global changes to simplecov output

For example, a quiet mode that silences all output, or a way to direct all simplecov output to a file. This seemed like a much more ambitious PR for me to make, and I don't know how useful others would find the functionality. We also do like some of simplecov's output, so a global configuration would be less useful for us.


Sorry for the lengthy comment! Does all of that make sense? Thoughts?

@PragTob
Copy link
Collaborator

PragTob commented Sep 25, 2019

@JacobEvelyn thanks a lot for the lengthy explanation, a bit more than I wanted but it helps turnaround time here! 😁 👍

Yeah the most important for me is the starting of sub processes that have to have simplecov enabled themselves in the black box tests. Although a bit odd I can see how it's a very valid approach (we're actually doing the same kinda just with the difference that we want to test simplecov).

I agree it's the simplest solution, supporting it going forward shouldn't be too complicated so I'm fine with having it 🎉

It'd be great though if you could add tests for it and document it in the README. If not, no worries but you'll have to wait for me to get around to doing it :)

@JacobEvelyn
Copy link
Contributor Author

Sure, I'll add some tests and documentation. Thanks for talking it through with me!

PR #688 added functionality that broke the
[`JacobEvelyn/friends`](https://github.com/JacobEvelyn/friends)
build (see https://travis-ci.com/JacobEvelyn/friends/jobs/238680478),
which runs many CLI processes and parses the output. Some CLI
processes are expected to have non-success status codes, but
these tests also check that our CLI wrote the correct message
to STDERR, and with #688 the output to STDERR is now changed.

This commit adds a SimpleCov configuration flag to disable the
message added in #688.
@JacobEvelyn
Copy link
Contributor Author

Okay @PragTob! I just added more changes to my commit here to put documentation in the README and add tests. Let me know what you think!

Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Those are even more tests than I would have wanted, I'd have probably just written the 2 unit tests and then one cucumber feature but it's also good like this.

Thanks for taking the time and contributing! 💚

image

@PragTob PragTob merged commit cb968ab into simplecov-ruby:master Sep 29, 2019
@JacobEvelyn
Copy link
Contributor Author

Thanks so much, @PragTob! Do you have a sense for when the next release will be?

@PragTob
Copy link
Collaborator

PragTob commented Oct 1, 2019

@JacobEvelyn not right now no, some of it depends on whether or not I get rubytogether funding to work on this. I'll try and re evaluate it this weekend, technically there's barely such a thing as too small releases. Would it be a big bother for you to use it from github in the mean time?

@JacobEvelyn
Copy link
Contributor Author

JacobEvelyn commented Oct 1, 2019

Unfortunately, since I'm using this in a gem I can't use it from GitHub because there's no way to specify GitHub dependencies in gemspec files. 😢 So sooner is better for me, but as a fellow OSS maintainer I know it's not always easy to find the time for these things—please don't feel pressure from me to rush!

@PragTob
Copy link
Collaborator

PragTob commented Oct 1, 2019

@JacobEvelyn you don't need to have development dependencies in the gemspec, you can just add them in the Gemfile as a normal gem entry and can then use github and what not :)

@JacobEvelyn
Copy link
Contributor Author

Ah neat, thanks!

JacobEvelyn added a commit to JacobEvelyn/friends that referenced this pull request Oct 3, 2019
This temporarily pins our simplecov dependency to the version
fixed by simplecov-ruby/simplecov#747 to fix
tests.

Fixes #238
JacobEvelyn added a commit to JacobEvelyn/friends that referenced this pull request Oct 3, 2019
This temporarily pins our simplecov dependency to the version
fixed by simplecov-ruby/simplecov#747 to fix
tests.

Fixes #238
JacobEvelyn added a commit to JacobEvelyn/friends that referenced this pull request Oct 3, 2019
This commit adds a `Gemfile.old` which is used for testing
old Ruby versions in Travis. This file does not include
dependencies such as Rubocop and SimpleCov that are only
used in either local development or our Travis build for
the latest Ruby version.

Included in these changes is pinning the SimpleCov dependency
to the version fixed by
simplecov-ruby/simplecov#747 to fix tests.

[@shen-sat](https://github.com/shen-sat) was instrumental in
discussing these changes and coming up with many parts of
the overall approach. Many thanks!

Fixes #238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants